Feature/bma/incr sync perf by bmarty · Pull Request #6917 · element-hq/element-android

bmarty · 2022-08-23T09:57:01Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Improve incremental sync performance by moving some work in the background.

Motivation and context

Fix #6027

other changes are regarding the fetch user mechanism, which is used to keep User database up to date.
Also some logs have been added.
13f7A9f has to be reviewed by the CryptoTeam (CC @BillCarsonFr ). This change is not strictly related to the performance issue, the work was already in BG. But I think the SDK were computing too many times the same thing.

A large incr sync is roughly passing from 70s to 8s here.

Screenshots / GIFs

Tests

Use the app with a big account, after a moment offline to have a big incremental sync. Observe that the progress bar is displayed a relatively short time.

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

…ker.

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryUpdater.kt

mnaturel · 2022-08-29T10:03:22Z

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryUpdater.kt

            if (roomSummaryEntity.isEncrypted && otherRoomMembers.isNotEmpty()) {
-                // mmm maybe we could only refresh shield instead of checking trust also?
-                crossSigningService.onUsersDeviceUpdate(otherRoomMembers)
+                if (aggregator == null) {


I struggle to understand why using the aggregator changes something here. Seeing the implementation of onUsersDeviceUpdate, I see we only schedule a worker we should not take so much time, right?

In the current code, crossSigningService.onUsersDeviceUpdate(otherRoomMembers) is called once per room.
With the new code this is called once per sync response (which can contains many rooms)

Okay thanks for explaining.

mnaturel · 2022-08-29T10:06:09Z

...trix/android/sdk/internal/session/sync/handler/SyncResponsePostTreatmentAggregatorHandler.kt

        private val updateUserAccountDataTask: UpdateUserAccountDataTask,
-        private val getProfileInfoTask: GetProfileInfoTask,
-        @SessionDatabase private val monarchy: Monarchy,
+        private val crossSigningService: DefaultCrossSigningService,


Why do we inject the DefaultCrossSigningService instead of the interface? Should the interface DeviceListManager.UserDevicesUpdateListener be implemented by CrossSigningService instead?

The arch is not really clean, we have exception for the crypto code.

BillCarsonFr

LGTM to me.
Maybe we could just do some renaming: We could rename onUsersDeviceUpdate in CrossSigningService in checkTrustAndAffectedRoomShiels.
And this new method would be called by the aggregator when some room membership as changed, and also as a DeviceListManager listener, when the user has some changes in keys (new device, new cross signing...)

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryUpdater.kt

mnaturel

Could not really test if sync is faster but okay for me.

bmarty · 2022-08-29T12:32:59Z

LGTM to me. Maybe we could just do some renaming: We could rename onUsersDeviceUpdate in CrossSigningService in checkTrustAndAffectedRoomShiels. And this new method would be called by the aggregator when some room membership as changed, and also as a DeviceListManager listener, when the user has some changes in keys (new device, new cross signing...)

Thanks. a8eb7d9

bmarty · 2022-08-30T14:50:17Z

The test is passing here...

sonarqubecloud · 2022-08-30T16:17:58Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

bmarty · 2022-08-31T07:06:25Z

This is not the same test failing. This time this is:

org.matrix.android.sdk.internal.crypto.E2eeSanityTests > testBasicBackupImport[test(AVD) - 9] FAILED

I am going to merge this PR as the fixes are highly expected.

bmarty added 5 commits August 22, 2022 17:45

Performance: invoke UpdateTrustWorker only once per incremental sync.

13f7a9f

Add some log for further investigation

9a0ea7b

Set instead of List, to avoid duplication.

a7666e2

Defer the treatment of updating the User profiles to a background Wor…

94a8774

…ker.

Do not fetch user if we do not have the previous content.

e3f5d15

bmarty requested review from a team, BillCarsonFr and mnaturel and removed request for a team August 23, 2022 09:57

bmarty added 4 commits August 23, 2022 11:58

changelog

c9e76f5

Fix logging issue

5c02290

Update comment

1a79828

typo

aa750cc

bmarty mentioned this pull request Aug 23, 2022

Fix crash when signing out. #6908

Merged

15 tasks

bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Aug 25, 2022

bmarty commented Aug 29, 2022

View reviewed changes

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryUpdater.kt Outdated Show resolved Hide resolved

mnaturel reviewed Aug 29, 2022

View reviewed changes

BillCarsonFr approved these changes Aug 29, 2022

View reviewed changes

bmarty commented Aug 29, 2022

View reviewed changes

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryUpdater.kt Outdated Show resolved Hide resolved

Remove tmp comment

f668be5

mnaturel approved these changes Aug 29, 2022

View reviewed changes

Create a new fun for code clarity

a8eb7d9

bmarty enabled auto-merge August 29, 2022 13:03

bmarty disabled auto-merge August 31, 2022 07:06

bmarty merged commit 456d831 into develop Aug 31, 2022

bmarty deleted the feature/bma/incr_sync_perf branch August 31, 2022 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/bma/incr sync perf#6917

Feature/bma/incr sync perf#6917
bmarty merged 11 commits intodevelopfrom
feature/bma/incr_sync_perf

bmarty commented Aug 23, 2022 •

edited

Loading

Uh oh!

Uh oh!

mnaturel Aug 29, 2022

Uh oh!

bmarty Aug 29, 2022

Uh oh!

mnaturel Aug 29, 2022

Uh oh!

mnaturel Aug 29, 2022

Uh oh!

bmarty Aug 29, 2022

Uh oh!

BillCarsonFr left a comment

Uh oh!

Uh oh!

mnaturel left a comment

Uh oh!

bmarty commented Aug 29, 2022

Uh oh!

bmarty commented Aug 30, 2022

Uh oh!

sonarqubecloud bot commented Aug 30, 2022

Uh oh!

bmarty commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bmarty commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

Uh oh!

mnaturel Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

bmarty Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

mnaturel Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

mnaturel Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

bmarty Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

BillCarsonFr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mnaturel left a comment

Choose a reason for hiding this comment

Uh oh!

bmarty commented Aug 29, 2022

Uh oh!

bmarty commented Aug 30, 2022

Uh oh!

sonarqubecloud bot commented Aug 30, 2022

Uh oh!

bmarty commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmarty commented Aug 23, 2022 •

edited

Loading